Fix Cosmos DB pagination for backwards compatibility#1594
Fix Cosmos DB pagination for backwards compatibility#1594MarcAstr0 wants to merge 5 commits intoboostercloud:mainfrom
Conversation
PR Summary
|
There was a problem hiding this comment.
Pull request overview
Restores backward compatibility for Azure/CosmosDB pagination by ensuring GraphQL cursors always include a numeric id while still supporting continuation-token pagination.
Changes:
- Add a default page size and revise Cosmos feed options to better support continuation tokens.
- Always include a numeric
cursor.id(and optionallycursor.continuationToken) in paginated results. - Update the monorepo lockfile and add a Rush change file for a patch release.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/framework-provider-azure/src/helpers/query-helper.ts | Adjusts pagination logic to return a numeric cursor id for backward compatibility and refines continuation-token/legacy OFFSET behavior. |
| common/config/rush/pnpm-lock.yaml | Updates workspace specifiers and dependency resolutions in the pnpm lockfile. |
| common/changes/@boostercloud/framework-core/fix-cosmos-pagination_2026-02-27-21-10.json | Adds a Rush change file to drive release/versioning metadata for the change. |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // cursor.id advances by the page size (limit) to maintain consistent page-based offsets | ||
| // that frontends rely on (e.g., limit=5 produces cursors 5, 10 ,15, ...) | ||
| const previousOffset = afterCursor?.id ? parseInt(afterCursor.id) : 0 | ||
| const effectiveLimit = limit ?? DEFAULT_PAGE_SIZE |
There was a problem hiding this comment.
previousOffset is computed with parseInt(afterCursor.id) without guarding for non-numeric values. If a client sends an unexpected/non-numeric id, this will become NaN and you’ll end up returning cursor.id: "NaN" (and also advance offsets incorrectly). Consider reusing the numeric check you already do for hasLegacyCursor (or fallback to 0 when Number.isNaN(parsed)), so cursor IDs are always valid numeric strings.
| // Always set maxItemCount when limit is provided or when using continuation token | ||
| if (limit || afterCursor?.continuationToken) { | ||
| feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE | ||
| } |
There was a problem hiding this comment.
When paginatedVersion is true and limit is omitted on the first page, effectiveLimit defaults to DEFAULT_PAGE_SIZE for the numeric cursor, but feedOptions.maxItemCount is not set (because there is no limit and no afterCursor.continuationToken). Cosmos may then return an arbitrary page size, making the returned cursor.id inconsistent with the number of items actually returned and potentially breaking the “id indicates position/page” assumption. Consider always setting feedOptions.maxItemCount to limit ?? DEFAULT_PAGE_SIZE whenever the continuation-token path is used (not only once a continuation token exists).
| // Always set maxItemCount when limit is provided or when using continuation token | |
| if (limit || afterCursor?.continuationToken) { | |
| feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE | |
| } | |
| // Always set maxItemCount for Cosmos pagination (defaulting when limit is omitted) | |
| feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE |
| let cursor: Record<string, string> | undefined | ||
| if (continuationToken) { | ||
| cursor = { continuationToken } | ||
| cursor = { continuationToken, id: (previousOffset + effectiveLimit).toString() } | ||
| } else if (finalResources.length > 0) { | ||
| const currentOffset = afterCursor?.id && !isNaN(parseInt(afterCursor.id)) ? parseInt(afterCursor.id) : 0 | ||
| cursor = { id: (currentOffset + finalResources.length).toString() } // Use the length of the results to calculate the next id | ||
| cursor = { id: (previousOffset + effectiveLimit).toString() } |
There was a problem hiding this comment.
The continuation-token pagination branch now returns a composite cursor ({ continuationToken, id }) and computes id using effectiveLimit. There are existing unit tests for search() in this package, but none appear to cover the continuation-token path (e.g., asserting maxItemCount/continuationToken usage and the returned cursor shape). Please add/update tests to cover: (1) first page continuation-token pagination (no afterCursor), (2) subsequent page with { continuationToken, id }, and (3) behavior when limit is omitted (default page size).
| { | ||
| "packageName": "@boostercloud/framework-core", | ||
| "comment": "Fix pagination issues with Cosmos DB", | ||
| "type": "patch" | ||
| } |
There was a problem hiding this comment.
This change file declares a patch for @boostercloud/framework-core, but the functional change in this PR is in @boostercloud/framework-provider-azure (Cosmos pagination logic). With Rush change files, this likely means the Azure provider won’t get a version bump/release containing the fix. Consider generating the change file for @boostercloud/framework-provider-azure (or whichever package actually ships query-helper.ts).
| [email protected]: | ||
| resolution: {integrity: sha512-DfXN8DfhJ7NH3Oe7cFmu3NCu1wKbkReJ8TorzSAFbSKrlNaQSKfIzqYqVY8zlbs2NLBbWpRiU52GX2PbaBVNkg==} | ||
| deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting [email protected] | ||
| hasBin: true | ||
|
|
||
| [email protected]: | ||
| resolution: {integrity: sha512-tvZgpqk6fz4BaNZ66ZsRaZnbHvP/jG3uKJvAZOwEVUL4RTA5nJeeLYfyN9/VA8NX/V3IBG+hkeuGpKjvELkVhA==} | ||
| engines: {node: 20 || >=22} |
There was a problem hiding this comment.
The lockfile introduces [email protected], which is marked as deprecated due to “widely publicized security vulnerabilities” in its own metadata. If possible, consider updating the dependency chain so a supported glob version is used (or confirm why this downgrade is required), since this can affect build tooling security posture.
| [email protected]: | |
| resolution: {integrity: sha512-DfXN8DfhJ7NH3Oe7cFmu3NCu1wKbkReJ8TorzSAFbSKrlNaQSKfIzqYqVY8zlbs2NLBbWpRiU52GX2PbaBVNkg==} | |
| deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting [email protected] | |
| hasBin: true | |
| [email protected]: | |
| resolution: {integrity: sha512-tvZgpqk6fz4BaNZ66ZsRaZnbHvP/jG3uKJvAZOwEVUL4RTA5nJeeLYfyN9/VA8NX/V3IBG+hkeuGpKjvELkVhA==} | |
| engines: {node: 20 || >=22} | |
| [email protected]: | |
| resolution: {integrity: sha512-tvZgpqk6fz4BaNZ66ZsRaZnbHvP/jG3uKJvAZOwEVUL4RTA5nJeeLYfyN9/VA8NX/V3IBG+hkeuGpKjvELkVhA==} | |
| engines: {node: 20 || >=22} | |
| hasBin: true |
|
/integration sha=04d10e1 |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
/integration sha=589add3 |
|
⌛ Integration tests are running... Check their status here 👈 |
|
✅ Integration tests have finished successfully! |
alvaroloes
left a comment
alvaroloes
left a comment
There was a problem hiding this comment.
Codex: I found a backward-compatibility regression in the new numeric cursor semantics. When limit is omitted, the implementation now treats the default page size as the cursor position, so a one-item page can emit { id: "100" }. A legacy client that feeds that numeric cursor back into the next request will skip rows instead of continuing from the next consumed item.
| } | ||
| } | ||
| if (process.env.TESTED_PROVIDER === 'AZURE' || process.env.TESTED_PROVIDER === 'LOCAL') { | ||
| expect(cartShippingAddress.cursor.id).to.equal('100') |
There was a problem hiding this comment.
Codex: This expectation is locking in the regression instead of the backward-compatibility guarantee from the PR description. For a paginated query that returns a single item with no explicit limit, the numeric cursor should still represent the next consumed position ("1"), not the default page size ("100"). Otherwise a legacy client that reuses { id: "100" } on the next request will skip 99 unseen rows.
Claude: Code Review — Fix Cosmos DB pagination for backwards compatibilityHe revisado los 4 ficheros cambiados en este PR. Análisis completo: Contexto y lógica del cambioEl PR #1587 introdujo paginación basada en ✅ Lo que está bien
🔴 Problemas que bloquean el merge1. const previousOffset = afterCursor?.id ? parseInt(afterCursor.id) : 0Si un cliente envía un
El código ya tiene esta guardia en otro lugar ( const parsedId = afterCursor?.id ? parseInt(afterCursor.id, 10) : NaN
const previousOffset = Number.isFinite(parsedId) ? parsedId : 02. // path con continuationToken:
cursor = { continuationToken, id: (previousOffset + effectiveLimit).toString() }
// path sin continuationToken (no legacy):
cursor = { id: (previousOffset + effectiveLimit).toString() }Cosmos DB no garantiza que devuelva exactamente
Si un cliente que solo usa La rama legacy del código antiguo ( Recomendación: en el path de } else if (finalResources.length > 0) {
cursor = { id: (previousOffset + finalResources.length).toString() }
}3. if (limit || afterCursor?.continuationToken) {
feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE
}En la primera llamada a una query paginada con continuationToken ( Esto hace que el // Siempre establecer maxItemCount en el path paginado
feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE🟡 Observaciones importantes4. Rush change file declara { "packageName": "@boostercloud/framework-core", ... }El código que cambia está en Los usuarios que actualicen Recomendación: Generar el change file con 5. Tests de integración fallan 4 veces antes de pasar — señal de flakiness El historial muestra 4 rondas de "Integration tests failed" antes de que pasen con el commit
Si es flakiness, debería abrirse un issue separado porque tests flaky en CI reducen la confianza en todos los PRs. 6. No hay tests unitarios para los nuevos paths Las observaciones de Copilot ya señalan esto: no hay tests unitarios que cubran:
Los tests de integración son lentos y caros para iterar sobre lógica de paginación. Los tests unitarios de 7. parseInt(afterCursor.id) // sin radixAunque en la práctica los IDs son siempre decimales, es buena práctica usar VeredictoRequest Changes. Los puntos 1 (NaN sin guardia), 2 (avance por |
|
/integration sha=50d87eb |
|
⌛ Integration tests are running... Check their status here 👈 |
|
✅ Integration tests have finished successfully! |
|
@alvaroloes implemented some of the suggestions in commit 50d87eb. Regarding the second one: The original code (commit 44045a7, before PR #1587) was: cursor: { id: ((limit ? limit : 1) + (afterCursor?.id ? parseInt(afterCursor?.id) : 0)).toString() }This always advanced by |
Description
v3.4.2 (see #1587) introduced continuation-token-based pagination for the Azure provider. That change replaced the numeric
idin the response cursor with acontinuationToken. For backwards compatibility,OFFSET-based pagination could still be forced by sending a numericidin theafterCursorproperty of the GraphQL request (e.g.,'0'for the first page). However, this broke existing frontends that didn't forceOFFSET-based pagination and expected a numericidin the cursor. This PR restores backwards compatibility by always returning a numeric cursorid(indicating the cursor's position) alongside the optionalcontinuationToken.Changes
query-helper.tsto always return a numericidin the cursor for backwards compatibilityChecks